[Bugfix] Fix broken bytecode dependency #14929
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 1h 19m total CI duration on this PR
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14929 +/- ##
=======================================
Coverage 60.1% 60.1%
=======================================
Files 856 856
Lines 210845 210905 +60
=======================================
+ Hits 126742 126788 +46
- Misses 84103 84117 +14 ☔ View full report in Codecov by Sentry. |
vgao1996
reviewed
Oct 18, 2024
There was a problem hiding this comment.
Okay, so I played with it and can confirm that it has fixed the create-object-and-publish-package workflow.
Left some comments on the implementation.
BTW I also discovered that aptos move unit-test would fail if you run it within a package with a bytecode dependency, so we should get that fixed as well. No need to include it in this PR though
Comment on lines
+650
to
+704
| let (file_map, all_compiled_units, optional_global_env) = | ||
| match config.compiler_version.unwrap_or_default() { | ||
| CompilerVersion::V1 => { | ||
| let mut paths = src_deps; | ||
| paths.push(sources_package_paths.clone()); | ||
| let compiler = Compiler::from_package_paths( | ||
| paths, | ||
| bytecode_deps.clone(), | ||
| flags, | ||
| &known_attributes, | ||
| ); | ||
| compiler_driver_v1(compiler)? | ||
| }, | ||
| version @ CompilerVersion::V2_0 | version @ CompilerVersion::V2_1 => { | ||
| let to_str_vec = |ps: &[Symbol]| { | ||
| ps.iter() | ||
| .map(move |s| s.as_str().to_owned()) | ||
| .collect::<Vec<_>>() | ||
| }; | ||
| let mut global_address_map = BTreeMap::new(); | ||
| for pack in std::iter::once(&sources_package_paths) | ||
| .chain(src_deps.iter()) | ||
| .chain(bytecode_deps.iter()) | ||
| { | ||
| for (name, val) in &pack.named_address_map { | ||
| if let Some(old) = | ||
| global_address_map.insert(name.as_str().to_owned(), *val) | ||
| { | ||
| if old != *val { | ||
| let pack_name = pack | ||
| .name | ||
| .map(|s| s.as_str().to_owned()) | ||
| .unwrap_or_else(|| "<unnamed>".to_owned()); | ||
| bail!( | ||
| "found remapped address alias `{}` (`{} != {}`) in package `{}`\ | ||
| , please use unique address aliases across dependencies", | ||
| name, old, val, pack_name | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| let mut options = move_compiler_v2::Options { | ||
| sources: sources_package_paths | ||
| .paths | ||
| .iter() | ||
| .map(|path| path.as_str().to_owned()) | ||
| .collect(), | ||
| sources_deps: src_deps.iter().flat_map(|x| to_str_vec(&x.paths)).collect(), | ||
| dependencies: bytecode_deps | ||
| .iter() | ||
| .flat_map(|x| to_str_vec(&x.paths)) | ||
| .collect(), | ||
| named_address_mapping: global_address_map | ||
| .into_iter() | ||
| .map(|(k, v)| format!("{}={}", k, v)) | ||
| .collect(), | ||
| skip_attribute_checks, | ||
| known_attributes: known_attributes.clone(), | ||
| language_version: Some(effective_language_version), | ||
| compiler_version: Some(version), | ||
| compile_test_code: flags.keep_testing_functions(), | ||
| experiments: config.experiments.clone(), | ||
| ..Default::default() | ||
| }; | ||
| options = options.set_experiment(Experiment::ATTACH_COMPILED_MODULE, true); | ||
| compiler_driver_v2(options)? | ||
| }, | ||
| }; | ||
| let mut options = move_compiler_v2::Options { | ||
| sources: sources_package_paths | ||
| .paths | ||
| .iter() | ||
| .map(|path| path.as_str().to_owned()) | ||
| .collect(), | ||
| sources_deps: src_deps.iter().flat_map(|x| to_str_vec(&x.paths)).collect(), | ||
| dependencies: bytecode_deps | ||
| .iter() | ||
| .flat_map(|x| to_str_vec(&x.paths)) | ||
| .collect(), | ||
| named_address_mapping: global_address_map | ||
| .into_iter() | ||
| .map(|(k, v)| format!("{}={}", k, v)) | ||
| .collect(), | ||
| skip_attribute_checks, | ||
| known_attributes: known_attributes.clone(), | ||
| language_version: Some(effective_language_version), | ||
| compiler_version: Some(version), | ||
| compile_test_code: flags.keep_testing_functions(), | ||
| experiments: config.experiments.clone(), | ||
| ..Default::default() | ||
| }; | ||
| options = options.set_experiment(Experiment::ATTACH_COMPILED_MODULE, true); | ||
| compiler_driver_v2(options)? | ||
| }, | ||
| }; |
There was a problem hiding this comment.
@fEst1ck it seems like these are just formatting changes. Can you confirm if this is indeed the case?
Comment on lines
159
to
170
| let mut bytecode_deps = vec![]; | ||
| for dep_name in self.package.bytecode_deps.iter().copied() { | ||
| let bytecode_paths = self.get_compiled_units_paths(dep_name)?; | ||
| let mut addrs = BTreeSet::new(); | ||
| for bytecode_path in bytecode_paths { | ||
| let addr = get_module_addr(dep_name, bytecode_path.as_str())?; | ||
| addrs.insert(addr); | ||
| } | ||
| for addr in addrs { | ||
| bytecode_deps.push((dep_name, addr)); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not so sure if the logic here is correct.
get_compiled_units_pathsdoesn't seem to have the ability to locate a bytecode dependency just by its name (without the address)- Similar to my other comment, there is no handling of duplicates
With that being said this function only seems to be referenced by some move-cli tests and it does not seem to be reachable from there either due to track_cov being set to false. Not sure if there's a good way to even test this.
fn run_all(args_path: &Path) -> datatest_stable::Result<()> {
let cli_exe = env!("CARGO_BIN_EXE_move");
let use_temp_dir = !args_path.parent().unwrap().join("NO_TEMPDIR").exists();
run_one(
args_path,
&PathBuf::from(cli_exe),
/* use_temp_dir */ use_temp_dir,
/* track_cov */ false,
)?;
Ok(())
}
There was a problem hiding this comment.
removed this part and left a TODO
| @@ -1138,3 +1173,23 @@ pub fn build_and_report_no_exit_v2_driver( | |||
| Some(env), | |||
| )) | |||
| } | |||
|
|
|||
| /// Returns the address of the module | |||
| fn get_module_addr(pkg_name: Symbol, pkg_path: &str) -> Result<NumericalAddress> { | |||
There was a problem hiding this comment.
I wonder if module_path would be a less confusing name
There was a problem hiding this comment.
fn get_package_module_addr or maybe fn get_addr_from_module_in_package might be a bit explicit with what this function does as well
Comment on lines
818
to
813
| bytecode_deps: bytecode_deps | ||
| .iter() | ||
| .flat_map(|package| { | ||
| let name = package.name.unwrap(); | ||
| package.paths.iter().map(move |pkg_path| { | ||
| get_module_addr(name, pkg_path.as_str()).map(|addr| (name, addr)) | ||
| }) | ||
| }) | ||
| .try_collect()?, |
There was a problem hiding this comment.
I get that you're trying to get the address of a compiled package, but the implementation seems a bit awkward
pkg_pathtechnically already contains the address of the package (e.g."/home/vgao/aptos-core/b/deps/0x8ae94b63db9b31761835258cd938ef670235069d37c17c616d06d28860dffe87/A/build/bytecode_modules/a.mv"), so you don't have to deserialize a module and then get the address from there.- The current code does not seem to handle bytecode packages with multiple modules
There was a problem hiding this comment.
- The package path part in the file structure
0x8ae94b63db9b31761835258cd938ef670235069d37c17c616d06d28860dffe87/A/build/bytecode_modules/a.mvis not required in the Move book. - changed field to
pub bytecode_deps: BTreeMap<PackageName, NumericalAddress>
wrwg
reviewed
Oct 21, 2024
| @@ -153,6 +156,18 @@ impl OnDiskCompiledPackage { | |||
| deps_compiled_units.push((dep_name, self.decode_unit(dep_name, &bytecode_path)?)) | |||
| } | |||
| } | |||
| let mut bytecode_deps = vec![]; | |||
There was a problem hiding this comment.
I'm a bit confused here -- isn't into_compiled_package only used by collect_coverage in the Move CLI? But the bug seems to be about deployment.
There was a problem hiding this comment.
This part is irrelevant to this fix. Removed.
Since testing this requires publishing a package with bytecode dependencies which is hard to be done automatically, we tested this maually.
vgao1996
approved these changes
Oct 29, 2024
There was a problem hiding this comment.
LGTM for now. Can you add a comment at the location where you deleted the following code as well, just for more clarity?
let mut bytecode_deps = vec![];
for dep_name in self.package.bytecode_deps.iter().copied() {
let bytecode_paths = self.get_compiled_units_paths(dep_name)?;
let mut addrs = BTreeSet::new();
for bytecode_path in bytecode_paths {
let addr = get_module_addr(dep_name, bytecode_path.as_str())?;
addrs.insert(addr);
}
for addr in addrs {
bytecode_deps.push((dep_name, addr));
}
}
rahxephon89
approved these changes
Oct 29, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
wrwg
approved these changes
Oct 29, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixed #14311. This PR continues the partial fix from @jasonxh #14340 by
OnDiskPackageThe original issue is caused by
CompiledPackagemissing information for bytecode dependencies, which propagates toPackageMetadatawhen constructing transaction payloads.How Has This Been Tested?
This is manually tested on the #14311
Type of Change
Which Components or Systems Does This Change Impact?